perf(desktop): consolidate keychain secrets into a single blob entry#1267
Merged
Conversation
787f073 to
a71bd5c
Compare
Buzz stored each secret as a separate keychain item — the identity key plus one entry per managed agent (6 agents = 7 items total). On dev builds (old keychain, null ACL) every read and write triggers a prompt: 15-20+ on first launch. On release builds (DPK) the first launch prompts once per key (7 total). Goose solved this identically: one entry, all secrets as a JSON map, one prompt total. Switch SecretStore to the same pattern. A single blob entry (service = the store's service name, username = "secrets") holds all keys as a JSON map. The map is cached in-memory after the first read, so subsequent probe/load/store/delete calls within the same process never touch the keychain again. One prompt on first launch, zero after. Migration: on first launch after upgrade the blob doesn't exist yet. load() falls back to reading the old per-key DPK entry (macOS) or legacy keyring entry (all platforms), writes it into the new blob, and deletes the old item — one-time per key, transparent to callers. The is_dpk_unavailable (-34018) fallback from #1266 is incorporated here: unsigned dev builds fall back to the legacy keyring crate for the single blob entry. The probe/load/store/delete semantics seen by callers (IdentityKeyStore, KeyStore) are unchanged. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
a71bd5c to
2e700a6
Compare
probe() only checked the new blob format, so users upgrading from per-key DPK (#1264) would get silent identity rotation — probe returned ReachableButEmpty, resolve_identity_with_store never called load(), and generate_and_persist created a new key. Add probe_legacy_key() to check old per-key DPK/keyring entries when the blob is absent. Also: save_blob() now updates the cache instead of invalidating it (avoids unnecessary keychain re-reads on sequential operations), non-availability errors in probe() map to ReachableButEmpty (matching old behavior instead of triggering fail-close), agent_secret_store() caches its SecretStore via OnceLock so the blob cache survives across call sites, Arc dropped from the cache field (SecretStore is never cloned), mutex locks recover from poisoning instead of panicking, and a migration integration test is added. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This was referenced Jun 25, 2026
wpfleger96
added a commit
that referenced
this pull request
Jun 25, 2026
Three more correctness fixes to the blob-based secret store: 1. mutate_blob() holds the cache mutex across the full read-from-keyring / mutate / write-to-keyring / update-cache sequence, closing the last-writer- wins race that persisted after SecretStore::shared() was added. 2. migrate_legacy_key() now checks for a DPK blob entry (key = "secrets") before checking per-key DPK entries, migrating anyone who ran main while #1267 was present and has a DPK blob instead of per-key entries. 3. Adds a with_cache() test constructor and a non-ignored probe() cache-hit test that runs in CI without the OS keychain. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Buzz stores each secret as a separate keychain item — the identity key plus one entry per managed agent (6 agents = 7 items total). On dev builds (old keychain, null ACL) every read and write triggers a prompt: 15–20+ on first launch. On release builds (DPK) the first launch prompts once per key (7 total). This is the root cause of the excessive prompting reported in the security channel.
Solution
Consolidate all secrets into a single JSON blob entry (service =
"buzz-desktop", username ="secrets"). One keychain access per process lifetime on read, one on write. One prompt on first launch, zero after — the same pattern Goose uses.SecretStoregains an in-memoryMutex<Option<HashMap<String,String>>>cache. After the first read the blob is cached; subsequentprobe/load/store/deletecalls within the same process never touch the keychain again.agent_secret_store()is cached viaOnceLockso the blob cache survives across call sites (previously each call constructed a freshSecretStorewith a cold cache).Migration
On first launch after upgrading from the per-key DPK format, the blob doesn't exist yet.
probe()checks old per-key DPK/keyring entries viaprobe_legacy_key()so callers that gateload()onPresent(likeresolve_identity_with_store) still trigger migration — without this,probe()would returnReachableButEmptyfor keys that exist only in the old format, causing silent identity rotation.load()then falls back to reading the old per-key DPK entry (macOS) or legacykeyringentry (all platforms), writes it into the new blob, and deletes the old item — one-time per key, transparent to callers. Agent keys that aren't found are re-populated from inline JSON byhydrate_keyson the same launch.Behavior after this change
tauri dev)Relationship to #1266
The
is_dpk_unavailable(-34018) fallback from #1266 is incorporated here — unsigned dev builds fall back to the legacykeyringcrate for the single blob entry. This PR supersedes #1266 for the prompt-reduction goal; #1266 still fixes the dev-build panic independently and should merge first (this PR is based onmainand is compatible either way).Stack: #1266 → this PR
Tests
Four integration tests are marked
#[ignore]— they require the real OS keychain and can't run in CI on unsigned builds. Run locally with:blob_stores_and_retrieves_multiple_keys— multi-key store/load round-tripblob_probe_present_absent_unreachable— probe semantics with and without blobblob_delete_removes_key_not_others— key isolation on deleteblob_migration_from_per_key_entry— end-to-end legacy key migration (seed old per-key entry → probe finds it → load migrates into blob → old entry deleted)Files changed
desktop/src-tauri/src/secret_store.rs— rewritten: blob storage, in-memory cache, legacy key probe/migrationdesktop/src-tauri/src/managed_agents/storage.rs—agent_secret_store()cached viaOnceLockdesktop/src-tauri/Cargo.lock—security-frameworkpromoted to direct dep (was already transitive)Caller interface
IdentityKeyStoreandKeyStoretraits inapp_state.rsandmanaged_agents/storage.rsare unchanged.probe/load/store/deletesemantics are preserved exactly.